Merged
Conversation
igaw
force-pushed
the
disable-fabrics
branch
3 times, most recently
from
4fd3c54 to
6bbfb96
Compare
With some more aggressive CFLAGS/LDFLAGS optimized for size the compiler
gets confused and can't figure out if ms is initialized or not.
../nvme.c:8567:20: error: ‘ms’ may be used uninitialized [-Werror=maybe-uninitialized]
8567 | if (ms && cfg.metadata_size < mbuffer_size)
| ^
../nvme.c:8356:15: note: ‘ms’ was declared here
8356 | __u16 ms;
| ^~
Signed-off-by: Daniel Wagner <[email protected]>
With some more aggressive CFLAGS/LDFLAGS optimized for size the compiler
gets confused and can't figure out if 'n' is initialized or not.
../libnvme/src/nvme/tree.c: In function ‘__nvme_scan_namespace’:
../libnvme/src/nvme/tree.c:2598:22: error: ‘n’ may be used uninitialized [-Werror=maybe-uninitialized]
2598 | n->sysfs_dir = path;
| ~~~~~~~~~~~~~^~~~~~
../libnvme/src/nvme/tree.c:2583:25: note: ‘n’ was declared here
2583 | struct nvme_ns *n;
| ^
Signed-off-by: Daniel Wagner <[email protected]>
The musl build fails due to a missing include:
In file included from ../unit/test-uint128.c:8:
../unit/../util/types.h:60:16: error: unknown type name ‘time_t’
60 | int convert_ts(time_t time, char *ts_buf);
| ^~~~~~
../unit/../util/types.h:11:1: note: ‘time_t’ is defined in header ‘<time.h>’; this is probably fixable by adding ‘#include <time.h>’
10 | #include <libnvme.h>
+++ |+#include <time.h>
11 |
Signed-off-by: Daniel Wagner <[email protected]>
igaw
force-pushed
the
disable-fabrics
branch
2 times, most recently
from
3bb0429 to
a3aedce
Compare
When the nvme_dump_config function arguments got updated, the no-json version was forgotten. Fixes: 74b30d2 ("json: update nvme_dump_config to a file descriptor") Signed-off-by: Daniel Wagner <[email protected]>
The nbft_info is defined in nbft thus move the linked list also there. This decouples the dependencies between nbft.h and fabrics.h Signed-off-by: Daniel Wagner <[email protected]>
The fabrics configuration data struct is always present, even the fabrics code is disabled. Thus the struct needs always to be initialized Signed-off-by: Daniel Wagner <[email protected]>
The hostid and hostnqn are used through out the library and are not fabric specific. Because tree.c depends on it move it to linux.c, so it's possible to make the fabrics code optional. Signed-off-by: Daniel Wagner <[email protected]>
The symbol versioning never really worked, thus it's adding unnecessary complexity. Thus the guarantee is that no existing symbols gets changed. Signed-off-by: Daniel Wagner <[email protected]>
The fabrics part is rather large and usually is not necessary for embedded use cases. Also it is very Linux specific and makes any porting attempts really hard. Thus make the fabrics part optional. Signed-off-by: Daniel Wagner <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR makes the NVMe-oF (fabrics) functionality optional, motivated by embedded use cases where fabrics support is unnecessary and its Linux-specific dependencies hinder porting. It separates the fabrics code path behind a new fabrics meson option (enabled by default), splits the linker script, moves host identity functions from fabrics.c to linux.c, and conditionally compiles fabrics-related commands, tests, and examples.
Changes:
- Introduces a
fabricsmeson feature option and gates fabrics source files, tests, examples, headers, and dependencies (liburing, openssl, keyutils, libdbus, python) on it. - Moves host identity functions (
hostnqn_generate,hostid_from_file, etc.) fromfabrics.ctolinux.cwith renamednvme_*prefixes (fromnvmf_*), and splits the linker script intolibnvme.ldandlibnvmf.ld. - Adds a
minimal_staticbuild configuration with aggressive size optimizations and fabrics disabled, along with corresponding CI workflow.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| meson_options.txt | Adds new fabrics feature option |
| meson.build | Gates dependencies and sources on want_fabrics |
| libnvme/src/meson.build | Splits sources/headers lists, adds libnvmf.ld, generates libnvme.h from template |
| libnvme/src/libnvme.h.in | Conditionally includes fabrics/nbft headers via @FABRICS_INCLUDE@ |
| libnvme/src/libnvme.ld | Removes fabrics symbols, bumps version to LIBNVME_3, adds new nvme_host* symbols |
| libnvme/src/libnvmf.ld | New linker script for fabrics symbols under LIBNVMF_3 |
| libnvme/src/nvme/linux.h | Declares new nvme_hostnqn_* / nvme_hostid_* functions |
| libnvme/src/nvme/linux.c | Moves host identity and UUID functions from fabrics.c |
| libnvme/src/nvme/fabrics.h | Removes moved function declarations, forward-declares nbft_file_entry |
| libnvme/src/nvme/fabrics.c | Removes moved functions and constants |
| libnvme/src/nvme/tree.c | Receives nvmf_default_config, renames calls to new API |
| libnvme/src/nvme/nbft.h | Moves nbft_file_entry struct definition here |
| libnvme/src/nvme/no-json.c | Updates json_update_config signature |
| nvme.c | Guards fabrics commands with #ifdef CONFIG_FABRICS, renames API calls, simplifies submit_io error handling |
| nvme-builtin.h | Guards fabrics ENTRY macros, moves dim entry inside guard |
| nvme-print-stdout.c | Guards fabrics-specific print functions, provides empty stubs when disabled |
| nvme-print-json.c | Guards fabrics-specific JSON print functions, provides empty stubs when disabled |
| util/cleanup.h | Guards _cleanup_uri_ behind CONFIG_FABRICS |
| util/types.h | Adds #include <time.h> |
| plugins/meson.build | Makes nbft plugin conditional on fabrics |
| scripts/build.sh | Adds minimal_static build config |
| scripts/release.sh | Removes ldscripts processing (now split) |
| .github/workflows/build.yml | Adds CI job for minimal_static build |
| libnvme/test/meson.build | Gates uriparser and nbft tests on fabrics |
| libnvme/test/ioctl/meson.build | Gates discovery test on fabrics |
| libnvme/examples/meson.build | Gates discover-loop example on fabrics |
| libnvme/libnvme/nvme.i | Updates SWIG bindings to use new function names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
igaw
force-pushed
the
disable-fabrics
branch
2 times, most recently
from
6709093 to
c4c95bc
Compare
Add a CI test for building without fabrics code. While at it also use some non standard CFLAGS and LDFLAGS. Signed-off-by: Daniel Wagner <[email protected]>
Use a local array for the CFLAGS to improve readability. Signed-off-by: Daniel Wagner <[email protected]>
Fix a couple of typos. Signed-off-by: Daniel Wagner <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The fabrics part is rather large and usually is not necessary for embedded use cases. Also it is very Linux specific and makes any porting attempts really hard. Thus make the fabrics part optional.